token based report #6216

pjain1 · 2024-12-05T11:22:52Z

Changes:

This PR changes all the report links to always have a magic token.
Magic tokens are created for each run of the report with expiry of 60 days for both internal and external users.
This enables the links to never expire and everytime its clicked it just works (if user is removed or added etc. in all scenarios) unless the token expires.

UI changes needed:

Report Creation API - Since even external users can now open the report in explore, the API now expects either the metrics view and explore name or canvas name on which report is created.
Open link - handle token and use it show the explore (similar to a public URL). If a user is logged in, it should display a banner to say "this is a preview link" or something like that (may need product input here). The report's token may have different permissions/attributes than the current user (e.g. if the report was created by an admin, but the recipient is not an admin), so it cannot simply show the dashboard as the current user.
Download link - use token to enable download, it already happens for external user now so probably only removing internal download page (if exists).
Unsubscribe link - It will need to grab the email or slack_user query param from the url and call the unsubscribe API with this param.

Important Note: This does not support locking time ranges as of now (locking dimension filter works), as they would be evaluated during each report run at runtime. The queries that are sent by explore have a separate time range apart from the filter, magic token only supports row filter. If mgc token has time range then it will need to be reconciled with the actual time range sent.

Contributes to https://github.com/orgs/rilldata/projects/38/views/8?pane=issue&itemId=85181742&issue=rilldata%7Crill-private-issues%7C855

begelundmuller

Also, do we have a job that clears expired magic auth tokens periodically? If not, can you add one?

admin/database/database.go

admin/server/reports.go

proto/rill/admin/v1/api.proto

pjain1 · 2024-12-16T16:49:04Z

Also, do we have a job that clears expired magic auth tokens periodically? If not, can you add one?

There is already a deleteExpiredAuthTokens job that will take care of it since we set an expiry.

admin/server/reports.go

begelundmuller · 2024-12-27T15:28:09Z

@pjain1 I responded to the two open questions on this PR. Can you let me know when you address them (or if you decide they are not worth it) and fix the merge conflict?

begelundmuller · 2025-01-09T12:46:07Z

admin/database/database.go

+type ResourceName struct {
+	Type string
+	Name string
 }


We should probably use snake case for JSON field names:

Suggested change

type ResourceName struct {

Type string

Name string

}

type ResourceName struct {

Type string `json:"type"`

Name string `json:"name"`

}

begelundmuller · 2025-01-09T12:46:19Z

admin/database/postgres/migrations/0057.sql

+                'Type', resource_type,
+                'Name', resource_name


Snake case:

Suggested change

'Type', resource_type,

'Name', resource_name

'type', resource_type,

'name', resource_name

begelundmuller · 2025-01-09T12:47:35Z

admin/database/database.go

 	FilterJSON      string
 	Fields          []string
 	State           string
 	DisplayName     string
 	Internal        bool
+	Resources       []ResourceName


nit: logically it feels like Resources should be placed here, like ResourceType and ResourceName were before:

Suggested change

FilterJSON string

Fields []string

State string

DisplayName string

Internal bool

Resources []ResourceName

Resources []ResourceName

FilterJSON string

Fields []string

State string

DisplayName string

Internal bool

begelundmuller · 2025-01-09T12:48:48Z

admin/server/magic_tokens.go

 		Filter:             filter,
 		Fields:             tkn.Fields,
 		State:              tkn.State,
 		DisplayName:        tkn.DisplayName,
+		Resources:          rs,


Same nit:

Suggested change

Filter: filter,

Fields: tkn.Fields,

State: tkn.State,

DisplayName: tkn.DisplayName,

Resources: rs,

Resources: rs,

Filter: filter,

Fields: tkn.Fields,

State: tkn.State,

DisplayName: tkn.DisplayName,

begelundmuller · 2025-01-09T13:05:48Z

admin/server/reports.go

 	}

-	externalUrls := make(map[string]*adminv1.GetReportMetaResponse_URLs, len(externalEmails))
-	emailTokens, err := s.reconcileMagicTokensForExternalEmails(ctx, proj.OrganizationID, proj.ID, req.Report, req.OwnerId, externalEmails)
+	tokens, ownerEmail, err := s.createMagicTokens(ctx, proj.OrganizationID, proj.ID, req.Report, req.OwnerId, recipients, req.Resources)


If a filter was applied to the report then that doesn't get propagated to the magic auth token here – so in theory, people can explore more data than that which the report filters by. Is that right? This is probably desirable in many cases, but not necessarily for external partners.

I wonder if we should perhaps add a flag to the report YAML and CreateReport API for whether or not to generate an open URL or only an export URL. E.g. it could be named open_mode: none|full (and then we could add support for a filtered mode later)

begelundmuller · 2025-01-09T13:06:19Z

admin/server/reports.go

+	if opts.Explore != "" && opts.Canvas != "" {
+		return nil, fmt.Errorf("cannot set both explore and canvas")
+	}
+	res.Annotations.Explore = opts.Explore
+	res.Annotations.Canvas = opts.Canvas


Should we check somewhere if the report has an explore or canvas? And if not, then skip generating the open link (since it would not actually work).

Some legacy reports and YAML-managed reports may not have an explore or canvas.

begelundmuller · 2025-01-09T13:07:45Z

proto/rill/admin/v1/api.proto

+  // list of resources to grant access to.
+  repeated ResourceName resources = 11;


nit: move up above resource_type and resource_name

begelundmuller · 2025-01-09T13:10:38Z

admin/server/magic_tokens.go

@@ -49,13 +47,19 @@ func (s *Server) IssueMagicAuthToken(ctx context.Context, req *adminv1.IssueMagi
 		return nil, status.Error(codes.PermissionDenied, "not allowed to create a magic auth token")
 	}

+	resources := make([]database.ResourceName, len(req.Resources))


It seems like it's missing backwards compatibiltiy for req.ResourceType and req.ResourceName? (Can be fixed by adding them to req.Resources if they are not empty)

token based report

32db47a

pjain1 marked this pull request as draft December 5, 2024 11:22

pjain1 added 4 commits December 5, 2024 16:54

merge

b0152c3

Merge branch 'main' into report_improvements

56a5ae8

store dependent resources in magic token

c090a2a

lint

abd2c57

pjain1 requested a review from begelundmuller December 9, 2024 15:42

pjain1 marked this pull request as ready for review December 9, 2024 15:43

begelundmuller requested changes Dec 10, 2024

View reviewed changes

review comments

4691567

pjain1 added 2 commits December 17, 2024 13:28

lint

92b919e

lint

8cbf0b8

pjain1 commented Dec 19, 2024

View reviewed changes

admin/server/reports.go Show resolved Hide resolved

pjain1 requested a review from begelundmuller December 19, 2024 06:42

pjain1 added 2 commits December 31, 2024 15:48

review comments

409a3f3

Merge branch 'main' into report_improvements

e215c70

begelundmuller requested changes Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

token based report #6216

token based report #6216

pjain1 commented Dec 5, 2024 •

edited by begelundmuller

Loading

begelundmuller left a comment

pjain1 commented Dec 16, 2024

begelundmuller commented Dec 27, 2024 •

edited

Loading

begelundmuller Jan 9, 2025

begelundmuller Jan 9, 2025

begelundmuller Jan 9, 2025

begelundmuller Jan 9, 2025

begelundmuller Jan 9, 2025

begelundmuller Jan 9, 2025

begelundmuller Jan 9, 2025

begelundmuller Jan 9, 2025

		// list of resources to grant access to.
		repeated ResourceName resources = 11;

token based report #6216

Are you sure you want to change the base?

token based report #6216

Conversation

pjain1 commented Dec 5, 2024 • edited by begelundmuller Loading

begelundmuller left a comment

Choose a reason for hiding this comment

pjain1 commented Dec 16, 2024

begelundmuller commented Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjain1 commented Dec 5, 2024 •

edited by begelundmuller

Loading

begelundmuller commented Dec 27, 2024 •

edited

Loading